Add network timing attributes to okhttp3 library #15664
Conversation
Signed-off-by: Surbhi Agarwal <[email protected]>
|
|
||
| dependencies { | ||
| library("com.squareup.okhttp3:okhttp:3.0.0") | ||
| library("com.squareup.okhttp3:okhttp:3.11.0") |
There was a problem hiding this comment.
A bit of a tangent to this PR, but I'd be surprised if folks are still using versions this old given it was released back in 2018, and would imagine most folks are using either 4 or 5.
There was a problem hiding this comment.
Should we create an issue to discuss and evaluate this and address it as needed in a separate PR?
There was a problem hiding this comment.
Should we create an issue to discuss and evaluate this and address it as needed in a separate PR?
In my opinion creating an issue won't help as this PR won't be merged before there is a decision on whether to increase the minimum supported version or not. Generally we are open to bumping the minimum supported version for library instrumetnations, but prefer to keep supporting old versions in the agent instrumentation. Here I suspect that bumping the version isn't really required since the instrumentation actually works with the old version, only the new network listener doesn't. You could avoid the discussion by replacing library("com.squareup.okhttp3:okhttp:3.0.0") with compileOnly("com.squareup.okhttp3:okhttp:3.11.0") and testLibrary("com.squareup.okhttp3:okhttp:3.0.0") and placing the network listener test into separate suite that tests with 3.11.0.
|
|
||
| @Override | ||
| public void callStart(Call call) { | ||
| Span.current().setAttribute(CALL_START, System.currentTimeMillis()); |
There was a problem hiding this comment.
This might not work when the event happens on a background thread. Are you sure that all of these callbacks are invoked so that the can see the current http client span?
|
|
||
| @Override | ||
| public void dnsStart(Call call, String domainName) { | ||
| Span.current().setAttribute(DNS_START, System.currentTimeMillis()); |
There was a problem hiding this comment.
The span start/end timestamps use nanoseconds, perhaps to be consistent with that and to be able to measure sub millisecond operations it would also make sense to use nanos here?
Also to accurately measure durations start and end time would need to be captured from a monotonic source.
There was a problem hiding this comment.
actually the span event itself already includes the event creation timestamp in nanoseconds so perhaps adding another timestamp isn't needed at all
| return "http"; | ||
| case SPDY_3: | ||
| return "spdy"; | ||
| default: |
There was a problem hiding this comment.
This was required as build was failing with error prone issue - Missing cases in enum switch
| } | ||
|
|
||
| @Override | ||
| public Call clone() throws CloneNotSupportedException { |
There was a problem hiding this comment.
why are these changes needed?
There was a problem hiding this comment.
In okhttp 3.11, CloneNotSupportedException is not thrown from the method, we still need to have the try catch block because super.clone can throw the exception.
| okhttp3.Request request = | ||
| new okhttp3.Request.Builder().url(resolveAddress("/success").toString()).build(); | ||
| okhttp3.Response response = | ||
| createCallFactoryWithNetworkTiming(new OkHttpClient.Builder()).newCall(request).execute(); |
There was a problem hiding this comment.
I think you should also test enqueue to verify that you get the expected events when request is running in a background thread.
| attrs.asMap().keySet().stream() | ||
| .map(AttributeKey::getKey) | ||
| .anyMatch( | ||
| k -> k.endsWith(".start_time") || k.endsWith(".end_time")); |
There was a problem hiding this comment.
This doesn't really verify much. I think you should assert all the events that you expect. This would also let you verify whether there are some events that are not recorded on the span.
|
|
||
| @Override | ||
| public void dnsStart(Call call, String domainName) { | ||
| Span.current().setAttribute(DNS_START, System.currentTimeMillis()); |
There was a problem hiding this comment.
will using Span.current() always ensure that this event is being added to the okhttp call tracing span?
It can happen that before this method call and after the okhttp trace span start some other span got started
4 participants
As discussed in the Add HTTP span events for network phases breakdown issue in semantic-convention repo, this PR resolves this issue in this repo.
Changes made in this PR:
NetworkTimingEventListenerfactory that captures the network phases breakdown timing as attributes in the current span.OkHttpTelemetry.newCallFactoryWithNetworkTiming) , the existing API and functionality remains unaffected.